Fix entitlement checks for relative links #124133

mosche · 2025-03-05T17:43:15Z

Entitlement checks are handling relative (symbolic) links incorrectly, such a path is resolved against the current work dir rather than the link's parent.

In Serverless this caused NotEntitledExceptions as the following, looking at it retrospectively the error is fairly obvious :/

Not entitled: component [(server)], module [org.elasticsearch.server], class [class org.elasticsearch.common.file.AbstractFileWatchingService], entitlement [file], operation [read], path [..data/settings.json]

This PR changes the checks for createSymbolicLink and createLink to resolve a relative link target correctly.

The check for toRealPath is changed from using readSymbolicLink to toRealPath. Besides above issue, readSymbolicLink does not work correctly in this case as it does not follow links. This is necessary with K8s mounts due to usage of a chain of symbolic links, e.g. mount-0.tmp -> ..data/mount-0.tmp -> ..version/mount-0.tmp where ..data is a symbolic link to ..version.

Manually following links is also not trivially possible, e.g. in above example ..data/mount-0.tmp is not a symbolic link (and readSymbolicLink would throw) making it an expensive task as each segment of a path needs to be checked.

Because of that I decided to switch to using toRealPath. As previously discussed in #122507, toRealPath has the disadvantage that the check on the target is skipped if it doesn't exist (throws a FileNotFoundException). Comparing the two options, this seems to be the lesser evil.

Relates to ES-11019

mosche · 2025-03-05T19:54:51Z

...itlement/qa/src/javaRestTest/java/org/elasticsearch/entitlement/qa/EntitlementsTestRule.java

                "files",
                List.of(
                    Map.of("path", tempDir.resolve("read_dir"), "mode", "read_write"),
+                    Map.of("path", tempDir.resolve("read_dir").resolve("k8s").resolve("..data"), "mode", "read", "exclusive", true),


..data is marked as exclusive here, it shouldn't ever be visible when testing K8s like file mounts (mount-0.tmp-> ..data/mount-0.tmp-> ..version/mount-0.tmp)

mosche · 2025-03-05T19:56:12Z

libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/PolicyManager.java

+        boolean canRead = entitlements.fileAccess().canRead(path);
+        if (canRead && followLinks) {
+            try {
+                realPath = path.toRealPath();


See PR description why using toRealPath

mosche · 2025-03-05T19:59:32Z

...src/main/java/org/elasticsearch/entitlement/runtime/api/ElasticsearchEntitlementChecker.java

-            }
-        }
-        policyManager.checkFileRead(callerClass, that);
+        policyManager.checkFileRead(callerClass, that, followLinks);


I moved the check into the PolicyManager, this allows to use toRealPath (without escaping the recursion here). See the PR description for why toRealPath instead of readSymbolicLink...

elasticsearchmachine · 2025-03-05T20:00:31Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

rjernst

Looks good, a couple minor questions

rjernst · 2025-03-05T20:54:30Z

...src/main/java/org/elasticsearch/entitlement/runtime/api/ElasticsearchEntitlementChecker.java


+    private static Path resolveLinkTarget(Path path, Path target) {
+        var parent = path.getParent();
+        return parent == null ? target : parent.resolve(target);


If target is absolute shouldn't we always return it? ie, should we only resolve relative to the parent when target is relative?

That's what parent.resolve(target) does. So didn't seem necessary to handle this case specially, but agreed it could make it easier to understand. Can do next week unless someone can pick this up

You are correct, it should not be strictly needed. I think this could be a follow-up if we feel we need it.

rjernst · 2025-03-05T20:56:36Z

libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/PolicyManager.java

+            try {
+                realPath = path.toRealPath();
+            } catch (IOException e) {
+                // target not found or other IO error


shouldn't we set canRead = false in this case?

In the ideal case we would throw a FileNotFoundException, as done by toRealPath in this case...
Not sure there's a good solution in this case, don't have a strong opinion...

In the worst case we leaking the information that a file does not exist in a location the caller shouldn't be able to access.

The opposite way the location might be OK, but the link broken... throwing a runtime exception the caller doesn't expect / handle in this case seems problematic as well 😔

Based on the ongoing discussion about tweaking exceptions we throw from instrumentation, I would lean towards throwing FileNotFoundException, but that would require changing the signature of our functions in the chain, right?
I don't think it should be too difficult, but it is additional work.
My 2c: I am ok with anything you decide is best for now, (leaving this as-is or setting canRead = false -- which would mean throwing NotEntitledException in the end), but I'd consider a follow-up that let us propagate the IOException / throw a FileNotFoundException.
Does it make sense?

I'll leave this as is and will follow up with a separate PR to throw a NoSuchFileException, it might spark additional discussions ... In the case of toRealPath it doesn't make a difference, the same exception would be thrown if calling toRealPath for real once the permission check passed.
That's also why keeping canRead = true is fine, the same would be thrown if attempting to read the content of the link.

ldematte

LGTM2, but I ++ both Ryan's questions

…hecking file entitlements (#124483) This will rethrow the `NoSuchFileException` when encountering an invalid symbolic link when following links during file (read) entitlement checks. Relates to #124133 (ES-11019)

…hecking file entitlements (elastic#124483) This will rethrow the `NoSuchFileException` when encountering an invalid symbolic link when following links during file (read) entitlement checks. Relates to elastic#124133 (ES-11019)

…hecking file entitlements (#124483) (#124543) This will rethrow the `NoSuchFileException` when encountering an invalid symbolic link when following links during file (read) entitlement checks. Relates to #124133 (ES-11019)

…hecking file entitlements (#124483) (#124541) This will rethrow the `NoSuchFileException` when encountering an invalid symbolic link when following links during file (read) entitlement checks. Relates to #124133 (ES-11019)

…hecking file entitlements (#124483) (#124542) This will rethrow the `NoSuchFileException` when encountering an invalid symbolic link when following links during file (read) entitlement checks. Relates to #124133 (ES-11019)

…hecking file entitlements (elastic#124483) This will rethrow the `NoSuchFileException` when encountering an invalid symbolic link when following links during file (read) entitlement checks. Relates to elastic#124133 (ES-11019)

mosche added auto-backport Automatically create backport pull requests when merged v8.18.1 v8.19.0 v9.0.1 v9.1.0 :Core/Infra/Entitlements Entitlements infrastructure labels Mar 5, 2025

Fix entitlement checks for relative links

eacba7c

mosche force-pushed the entitlements/fix-relative-links branch from fa92815 to eacba7c Compare March 5, 2025 19:05

mosche commented Mar 5, 2025

View reviewed changes

mosche marked this pull request as ready for review March 5, 2025 20:00

mosche requested a review from a team as a code owner March 5, 2025 20:00

elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Mar 5, 2025

mosche added >non-issue >refactoring and removed >non-issue labels Mar 5, 2025

rjernst approved these changes Mar 5, 2025

View reviewed changes

ldematte approved these changes Mar 6, 2025

View reviewed changes

Merge branch 'main' into entitlements/fix-relative-links

ee2b3ef

mosche merged commit 7cec948 into elastic:main Mar 10, 2025
17 checks passed

mosche deleted the entitlements/fix-relative-links branch March 10, 2025 12:45

mosche mentioned this pull request Mar 10, 2025

[8.18] Fix entitlement checks for relative links (#124133) #124478

Merged

mosche added a commit to mosche/elasticsearch that referenced this pull request Mar 10, 2025

Fix entitlement checks for relative links (elastic#124133)

fe90f1c

mosche mentioned this pull request Mar 10, 2025

[8.x] Fix entitlement checks for relative links (#124133) #124479

Merged

mosche added a commit to mosche/elasticsearch that referenced this pull request Mar 10, 2025

Fix entitlement checks for relative links (elastic#124133)

910b8bf

mosche mentioned this pull request Mar 10, 2025

[9.0] Fix entitlement checks for relative links (#124133) #124480

Merged

mosche added a commit to mosche/elasticsearch that referenced this pull request Mar 10, 2025

Fix entitlement checks for relative links (elastic#124133)

8dd493f

mosche mentioned this pull request Mar 10, 2025

Rethrow NoSuchFileException if encountering an invalid symlink when checking file entitlements #124483

Merged

elasticsearchmachine pushed a commit that referenced this pull request Mar 10, 2025

Fix entitlement checks for relative links (#124133) (#124480)

b098aca

elasticsearchmachine pushed a commit that referenced this pull request Mar 10, 2025

Fix entitlement checks for relative links (#124133) (#124478)

155ba49

elasticsearchmachine pushed a commit that referenced this pull request Mar 10, 2025

Fix entitlement checks for relative links (#124133) (#124479)

7c6cce0

georgewallace pushed a commit to georgewallace/elasticsearch that referenced this pull request Mar 11, 2025

Fix entitlement checks for relative links (elastic#124133)

80c1b86

jfreden pushed a commit to jfreden/elasticsearch that referenced this pull request Mar 13, 2025

Fix entitlement checks for relative links (elastic#124133)

6d453e9

Fix entitlement checks for relative links #124133

Fix entitlement checks for relative links #124133

Uh oh!

Conversation

mosche commented Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Mar 5, 2025

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldematte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mosche commented Mar 5, 2025 •

edited

Loading